Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rosetta): extract does not respect strict metadata entry #2863

Merged
merged 7 commits into from
Jun 29, 2021

Conversation

BenChaimberg
Copy link
Contributor

@BenChaimberg BenChaimberg commented May 28, 2021

The jsii.rosetta.strict assembly metadata entry was not being respected by the extract command due to two bugs:

  1. Fixturizing (the process by which snippets are enriched by a separate fixture) did not transfer the "strict" flag from the original snippet to the fixturized snippet. Fix: transfer the strict flag to the fixturized snippet.
  2. Translation of snippets to other languages using the worker_threads node module involves sending compiler diagnostics across the wire. These diagnostics should be annotated with a "strict brand" that marks them as failing compilation and allows further steps to fail if such a diagnostic exist. Because the brand was a Symbol, it is not correctly serialized. Fix: make the brand a string so it can be properly serialized.

fixes #2861


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 28, 2021
@BenChaimberg
Copy link
Contributor Author

I opted for a simpler fix than described in the issue: just changed the "strict brand" from a symbol to a string with a "jsii" prefix

@BenChaimberg BenChaimberg marked this pull request as ready for review June 1, 2021 20:56
@BenChaimberg BenChaimberg requested review from RomainMuller and a team June 1, 2021 20:57
packages/jsii-rosetta/lib/util.ts Outdated Show resolved Hide resolved
packages/jsii-rosetta/test/util.test.ts Outdated Show resolved Hide resolved
@BenChaimberg BenChaimberg requested review from RomainMuller and a team June 9, 2021 21:58
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you summarize the issue and the fix in the PR description?
Would be nice to get that into the git commit message with the ability to read directly on the IDE/console, rather than have to click into a Github issue.

There's a ton of material you'll find on the internet on the value of good commit messages. Here's one that's standard token - https://medium.com/swlh/why-should-i-write-good-commit-messages-e15d37bf45cb.

Change look good to me.

Added 'do-not-merge' in case @RomainMuller wants to take another look.

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Jun 10, 2021
@BenChaimberg BenChaimberg removed the pr/do-not-merge This PR should not be merged at this time. label Jun 11, 2021
@BenChaimberg
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 11, 2021

Command refresh: success

Pull request refreshed

@BenChaimberg BenChaimberg removed the request for review from RomainMuller June 11, 2021 12:40
@BenChaimberg
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 11, 2021

Command refresh: success

Pull request refreshed

@mergify
Copy link
Contributor

mergify bot commented Jun 11, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Jun 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Jun 11, 2021

Merging (with squash)...

@BenChaimberg
Copy link
Contributor Author

BenChaimberg commented Jun 16, 2021

@RomainMuller for some reason, the jsii-pacmak test:build is failing with error:

jsii-pacmak: #STDERR> /home/runner/work/jsii/jsii/packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/process/handshake.go:7:2: missing go.sum entry for module providing package github.com/Masterminds/semver/v3 (imported by github.com/aws/jsii-runtime-go/internal/kernel/process); to add:
jsii-pacmak: #STDERR> go get github.com/aws/jsii-runtime-go/internal/kernel/process@v0.0.0

I don't understand why this would suddenly be a problem and why other builds are succeeding. The go.sum in question does in fact have entries for Masterminds/semver/v3. I've run the tests locally in the superchain image and there aren't any problems.

@RomainMuller
Copy link
Contributor

The build problem is apparently caused by a bug in go mod download in the 1.16 versions...

#2893

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2021

Merging (with squash)...

@mergify mergify bot merged commit 5d2392b into main Jun 29, 2021
@mergify mergify bot deleted the chaimber/fix_strict branch June 29, 2021 15:37
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jun 29, 2021
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Jul 1, 2021
A bugfix in jsii-rosetta (aws/jsii#2863) now enforces the `jsii.rosetta.strict` jsii assembly metadata entry, meaning modules with `jsii.metadata.jsii.rosetta.strict` set to `true` in their `package.json` will need their snippets to compile or builds will fail. Because this entry was not respected previously, two modules (`core` and `aws-iam`) have snippets that fail to compile. This means that future builds will start failing once the changes to jsii are released. Further, jsii integration tests are failing due to this breakage.

This change simply disables strict compilation for these modules since fixed snippets may be overwritten before the jsii changes are released, causing future builds to fail.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
A bugfix in jsii-rosetta (aws/jsii#2863) now enforces the `jsii.rosetta.strict` jsii assembly metadata entry, meaning modules with `jsii.metadata.jsii.rosetta.strict` set to `true` in their `package.json` will need their snippets to compile or builds will fail. Because this entry was not respected previously, two modules (`core` and `aws-iam`) have snippets that fail to compile. This means that future builds will start failing once the changes to jsii are released. Further, jsii integration tests are failing due to this breakage.

This change simply disables strict compilation for these modules since fixed snippets may be overwritten before the jsii changes are released, causing future builds to fail.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rosetta extract command doesn't respect strict metadata entry
3 participants